Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly handle missing dune-project #227

Merged
merged 5 commits into from
Nov 8, 2021
Merged

Properly handle missing dune-project #227

merged 5 commits into from
Nov 8, 2021

Conversation

NathanReb
Copy link
Contributor

@NathanReb NathanReb commented Nov 5, 2021

Fixes #225 !

Depends on #203.

This PR improves the error message when a dune-project file is missing at the root. As the test shows, it now prints a nice error with suggested actions instead of an uncaught exception with a backtrace!

@Leonidas-from-XIV
Copy link
Member

Just merged #203, can you please rebase? That will make reviewing a bit easier.

Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This definitely brings opam-monorepo closer to where we want to be. I'm actually more excited about the first cram test than then fix itself. But we need to make sure the test output is less fragile.

lib/project.mli Show resolved Hide resolved
test/bin/missing-dune-project/run.t Outdated Show resolved Hide resolved
CHANGES.md Show resolved Hide resolved
The name of the lockfile to generate depends on the target packages
rather than on the local ones. This both clarifies that by renaming
the labeled argument, making it mandatory and removing the unused
code path where it would try to determine the local packages itself.

Signed-off-by: Nathan Rebours <[email protected]>
Copy link
Member

@Leonidas-from-XIV Leonidas-from-XIV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest commit the tests look fine to me. One can consider doing something with the error reporting to exit directly instead but I wouldn't delay this PR to find the most theoretically beautiful solution.

cli/lock.ml Show resolved Hide resolved
test/bin/missing-dune-project/run.t Outdated Show resolved Hide resolved
test/bin/missing-dune-project/dune Outdated Show resolved Hide resolved
This moves the dune file up so it will apply to all future tests
as long as turning the test into a cram folder, incuding the files
to the tree rather than generating them as part of the test.

`cd`ing into the folder to try running commands is now possible which
should help debugging issues.
We still `cat` the content of the files so that the `run.t` file is self
contained and one does not have to browse files to fully understand the
test.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Copy link
Contributor Author

I moved the test to a cram folder an wrote the file to the source tree instead of generating them. I think that's a better practice for writing tests.

I still cat the files in run.t so that it's self contained and no file browsing is required to understand the test.

cding inside the test folder and running dune exec -- opam-monorepo ... now makes for a better dev experience if one wants to reproduce or debug.

@NathanReb NathanReb merged commit 92432ff into main Nov 8, 2021
@Leonidas-from-XIV Leonidas-from-XIV deleted the no-dune-project branch November 8, 2021 15:37
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Nov 23, 2021
CHANGES:

### Added

- Add a list subcommand to list the duniverse packages in the lockfile
  (tarides/opam-monorepo#217, @samoht)

### Changed

- Only warn users about missing dune-ports repo in OPAM switch if no solution
  can be found due to packages not building with dune (tarides/opam-monorepo#210, @Leonidas-from-XIV)
- Rename the `--repo` option to `--root` to make it more
  straightforward  that this is referring to the project root (tarides/opam-monorepo#218, @samoht)
- Improve the wording of the lockfile selection log (tarides/opam-monorepo#222, @NathanReb)
- Display the full solver error with `--verbose` (tarides/opam-monorepo#229, @emillon)

### Fixed

- Better errors for `opam-monorepo depext`, especially for non-interactive
  shells (tarides/opam-monorepo#216, @samoht)
- Properly detect all opam packages defined in the current repository, preventing it
  from later pulling duplicates into the duniverse if they were part of the target packages
  dependencies. (tarides/opam-monorepo#203, @Leonidas-from-XIV)
- Properly report missing dune-project file when trying to determine the
  to-be-genrated lockfile name (tarides/opam-monorepo#227, @NathanReb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running opam-monorepo in a project without dune-project file at the root crashes
3 participants